Improve AliBuild typing and linter strictness #882
Conversation
alibuild_helpers/sync.py
Outdated
| @@ -233,7 +218,7 @@ def fetch_symlinks(self, spec): | |||
| os.path.join(self.workdir, links_path, linkname)) | |||
|
|
|||
| def upload_symlinks_and_tarball(self, spec): | |||
| pass | |||
| dieOnError(True, "HTTP backend does not support uploading directly") | |||
There was a problem hiding this comment.
Is this change correct? The tests seem to suggest that it should fail silently, instead of crashing like how the CVMFS backend fails
singiamtel
force-pushed
the
improve_typing
branch
from
70c9385 to
2320045
Compare
alibuild_helpers/sync.py
Outdated
| destFp = None | ||
| try: | ||
| destFp = open(dest+".tmp", "wb") if dest else None | ||
| if dest: destFp = open(dest+".tmp", "wb") |
There was a problem hiding this comment.
I did this one so destFp is always bounded on the finally block, otherwise if open() throws aliBuild would crash trying to access it
There was a problem hiding this comment.
is the finally actually needed / working? is there a case where open throws and still returns destFp? Maybe we should fix that.
There was a problem hiding this comment.
maybe we should split the opening of the file from the rest of the try catch...
There was a problem hiding this comment.
I would live this particular change out and continue with the rest of the PR, since to that I have no particular objections.
There was a problem hiding this comment.
Okay, leaving it out
singiamtel
force-pushed
the
improve_typing
branch
from
52650d4 to
c5f2c63
Compare
singiamtel
force-pushed
the
improve_typing
branch
from
f6473d7 to
ad3935e
Compare
singiamtel
changed the title
singiamtel
force-pushed
the
improve_typing
branch
7 times, most recently
from
b34d42e to
cd0d961
Compare
singiamtel
force-pushed
the
improve_typing
branch
from
cd0d961 to
e3cccf7
Compare
|
I don't understant the CI error at all. Apparently they changed the default output of argparse? But somehow our test suite is getting both, and fails no matter which one I try As a workaround I could either make the test laxer, or make our own argparse Formatter class so at least it's consistent (https://docs.python.org/3/library/argparse.html#formatter-class) It really is impressive how often Python breaks stuff... |
|
Otherwise this is ready for merge, I want to make more changes but this PR is already very big, so it's probably best to keep it simple |
I'm currently trying to type the codebase as much as possible, and use a stronger mypy filter.
Not ready to merge yet, just want to get the tests running